-
Notifications
You must be signed in to change notification settings - Fork 2
feat(be): WebSocket 인증 작업 #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatmessage/controller/ChatMessageController.kt
🟢 좋은점:
- ApiResponse 사용: listMessages와 sendMessage 메서드 모두 ResponseEntity<ApiResponse> 형태로 응답을 감싸서 반환하므로, 표준 응답 형식을 잘 준수하고 있습니다. 이는 API의 일관성을 유지하는 데 기여합니다.
- Kotlin 최적화: null safety를 적절히 활용하여 @AuthenticationPrincipal로 받은 requesterId와 senderId를 Long?으로 처리하고, null 체크 후 예외를 발생시킵니다. 이는 런타임 오류를 방지합니다. 또한 ChatMessageResponse::from과 같은 확장 함수(또는 팩토리 메서드)를 사용해 매핑을 간결하게 처리합니다.
- ktlint 규칙: 코드 포맷팅이 깔끔하며, 네이밍 컨벤션이 대체로 camelCase를 따르고 있습니다. import 문도 필요한 것만 추가되어 불필요한 중복이 없습니다.
🟡 개선사항:
- Kotlin 최적화: if-else 블록으로 messages를 가져오는 부분을 when 표현식으로 리팩토링하면 더 Kotlin다운 코드가 될 수 있습니다. 예:
val messages = when { after == null -> messageService.getlistbefore(roomId, limit, memberId); else -> messageService.getlistafter(roomId, after, memberId) }. 이는 가독성을 높입니다. - ktlint 규칙: 서비스 메서드 이름(getlistbefore, getlistafter)이 camelCase이지만 'list'가 붙어 일관성 있게 보이지 않습니다. getListBefore, getListAfter처럼 PascalCase나 더 명확한 네이밍(예: getMessagesBefore, getMessagesAfter)으로 변경하면 서비스 계층의 컨벤션을 강화할 수 있습니다. 또한 limit 파라미터에 대한 기본값(50) 외에 최대값 검증(예: @Min/@max 어노테이션 추가)을 고려하세요.
- 전반적: @AuthenticationPrincipal의 타입을 Long?으로 직접 바인딩하는 대신, SecurityContextHolder나 커스텀 UserDetails에서 ID를 추출하는 헬퍼 메서드를 컨트롤러나 인터셉터에 추가하면 재사용성이 높아집니다. sendMessage에서 req.senderId를 완전히 제거한 것은 보안상
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatmessage/controller/ChatMessageSocketController.kt
🟢 좋은점:
- Kotlin의 null safety를 잘 활용하여
principal.name.toLongOrNull()로 안전하게 Long 변환을 처리하고, 실패 시 예외를 던지는 로직이 적절합니다. 이는 Kotlin 최적화 규칙( null safety)을 준수합니다. - Principal을 통해 인증된 사용자 ID를 추출하는 방식으로 보안을 강화한 점이 좋습니다. 이는 기존 req.senderId 직접 사용보다 안전합니다.
- ktlint 규칙 측면에서 네이밍 컨벤션(예: handleMessage, senderId 등 camelCase)이 일관되며, 코드 포맷팅이 간결하고 읽기 쉽습니다.
🟡 개선사항:
- Kotlin 최적화 측면에서
toLongOrNull()후 throw 대신when표현식을 사용해 더 명시적인 분기 처리(예: when(val id = principal.name.toLongOrNull()) { null -> throw ...; else -> ... })를 고려할 수 있습니다. 이는 코드 가독성을 높일 수 있습니다. - WebSocket 컨트롤러이지만, 응답 시
ChatMessageResponse.from(saved)를 직접 convertAndSend로 보내는 부분에서 ApiResponse 래퍼를 적용하면(가능하다면) 일관된 응답 형식을 유지할 수 있습니다. 규칙상 "모든 API"에 적용되므로, WebSocket도 예외가 아닌 경우 확장 함수로 ApiResponse를 감싸는 방식을 제안합니다. - 글로벌 익셉션 처리(@ControllerAdvice)가 WebSocket 핸들러에도 적용되는지 확인하세요. AccessDeniedException을 직접 throw하는 대신, 커스텀 예외(예: AuthenticationRequiredException)를 만들어 @ControllerAdvice에서 표준 에러 응답으로 처리하면 더 일관적입니다.
🔴 문제점:
- ApiResponse 사용 규칙을 위반합니다. WebSocket 응답(
messagingTemplate.convertAndSend)이 ApiResponse로 감싸지지 않았습니다. 모든 API(포함 WebSocket 핸들러)에 적용해야 하므로, 응답 객체를 ApiResponse로 래핑하여 보내야 합니다. - 글로벌 익셉션 처리 측면에서 AccessDeniedException을 직접 throw하지만, WebSocket 컨텍스트에서 @ControllerAdvice가 제대로 동작하지 않을
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 AI 리뷰 - src/main/kotlin/com/back/koreaTravelGuide/domain/userChat/chatmessage/dto/ChatMessageSendRequest.kt
🟢 좋은점:
- Kotlin 최적화 측면에서 data class를 적절히 사용하고 있으며, 필드가 간단하고 null safety가 보장된 non-null 타입(String)으로 구성되어 있어 불필요한 null 체크가 최소화됨. senderId 필드 제거로 DTO가 더 간결해져 API 요청의 가벼움 측면에서 긍정적임 (클라이언트 측에서 senderId를 직접 전달하지 않도록 설계된 것으로 보임, 이는 인증 기반 자동 설정을 유도할 수 있음).
🟡 개선사항:
- senderId 필드 제거로 인해 해당 DTO를 사용하는 컨트롤러나 서비스 로직(예: 채팅 메시지 저장 시 sender 처리)에서 현재 인증된 사용자 ID를 자동으로 주입하는 부분이 제대로 구현되었는지 확인 필요. 만약 클라이언트에서 senderId를 생략하도록 API 문서화가 업데이트되지 않았다면, Swagger나 API 스펙에 반영하여 혼란을 방지하는 것이 좋음. ktlint 규칙 준수 여부는 변경이 최소화되어 문제없으나, 전체 파일의 네이밍 컨벤션(예: camelCase 유지)을 재확인 추천.
🔴 문제점:
- 없음. 변경이 의도된 DTO 단순화로 보이며, 글로벌 익셉션 처리나 ApiResponse 규칙과 직접 충돌하지 않음. 다만, senderId가 보안/인증 로직에서 필수적이었다면 후속 PR에서 관련 테스트 케이스 추가를 권장.
|
확인 하였습니다! |
YangHJ2415
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인하고 승인하였습니다.
Work Details
1.Enforce JWT-backed authentication across user chat REST and STOMP endpoints, deriving user IDs from principals and rejecting unauthenticated requests
2.Update chat service methods with clearer naming and requester context.
3.Refactor: Clean up unused imports
To Do: RabbitMQ integration and Refactoring